Skip to content

feat(bank): integrate Bank Frick WebAPI#4196

Merged
TaprootFreak merged 20 commits into
developfrom
agent/integrate-bank-frick
Jul 15, 2026
Merged

feat(bank): integrate Bank Frick WebAPI#4196
TaprootFreak merged 20 commits into
developfrom
agent/integrate-bank-frick

Conversation

@joshuakrueger-dfx

@joshuakrueger-dfx joshuakrueger-dfx commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

Implements #4193 as a default-off Bank Frick WebAPI rail:

  • registers Bank Frick in configuration, infrastructure, the bank registry, local seed data and a production data migration with unmistakably synthetic, disabled account placeholders
  • imports signed camt.053 statements into bank_tx with strict parsing, deterministic references, per-account cursors and existing dedup/assignment behavior
  • creates idempotent EUR/CHF JSON payment orders, supports automatic or portal/TAN approval, polls the complete Frick lifecycle and reconciles booked debits to exactly one fiat output
  • adds dedicated payout identity/status/error fields, safe routing/liquidity rules and an activation/backfill runbook
  • keeps both Frick processes disabled and all placeholder bank rows receive=false, send=false, sctInst=false until Operations supplies and verifies the private account data

Security and correctness

  • signs the exact serialized request bytes with RSA-SHA512, including authorization and bodyless GET requests
  • caches JWTs, coalesces concurrent authorization and performs at most one controlled 401 re-authorization
  • verifies the detached signature over the exact raw response before parsing, accepting only the configured RSA SHA-512/384/256 algorithms and the separately configured FRICK_SERVER_PUBLIC_KEY
  • fails closed without credentials, response-verification material, required headers, supported algorithms or structurally valid CAMT content
  • excludes secrets, tokens and raw bank responses from operational errors
  • uses a stable customId, semantic collision checks and fromDate=1970-01-01 on every active and BOOKED recovery lookup
  • advances lastBankFrickDate:<bankId> only after a non-empty response was fully persisted, to max(previous, min(now, latest booking date) - 2 days); the update is atomic and monotonic, while empty, malformed and partially persisted responses never advance it
  • requires an unambiguous outgoing match across account, amount, currency, debit direction, readiness and reference before completing a payout
  • keeps liquidity reserved for every nonterminal state, including DELETION_REQUESTED
  • decouples approval of existing PREPARED orders from the new-payment transmission kill switch
  • fails routing closed when Frick is unavailable or a BIC/bank selection is ambiguous

TaprootFreak review follow-up

All seven inline findings are implemented and regression-tested:

  1. every customId lookup uses a wide fromDate
  2. the CI coverage gate covers all seven isolated Frick surfaces at 100% statements/branches/functions/lines
  3. cursor advancement uses the issue-required non-empty rule, latest-booking-date overlap and an atomic monotonic update; rewind/backfill is documented
  4. DELETION_REQUESTED remains polled and retains liquidity
  5. status-job approval no longer depends on the transmission flag
  6. shared CAMT rollout monitoring and one-time Raiffeisen/Yapeal reference-less duplicate cleanup are documented
  7. the customer number is validated as 1–7 digits

The previously raised SEPA-Instant express concern remains correctly withdrawn: Bank Frick's documented SEPA_INSTANT request omits that field.

Explicit implementation decisions

  • inbound source: booked camt.053
  • outbound source: PUT /transactions, allowing EUR SEPA/SEPA_INSTANT and CHF FOREIGN with mandatory SHA charge handling
  • approval: automatic signTransactionWithoutTan only when explicitly enabled; otherwise the order remains PREPARED for portal/TAN approval and continues to be polled
  • account model: one disabled CHF and one disabled EUR placeholder; concrete IBANs, CT/Operating roles, sender priority, instant capability and liquidity assets are operational inputs, not guessed in code
  • no frontend or public API contract changes
  • optional notification webhooks remain out of scope; polling is the source of truth

Validation for 77eb99e13

  • GitHub CI: 8/8 checks successful (0 failing, 0 pending), including migration immutability, both CodeQL workflows and the PR Review Bot
  • current-head remote pipeline: install, lint, format, production build, full Jest run with PostgreSQL, exact Frick coverage gate and critical security audit
  • current-head local checks: npm run format:check, npm run lint, npm run type-check
  • current-head focused Frick run: 7 suites / 226 tests passed; exact 100% statements (689/689), branches (530/530), functions (98/98) and lines (597/597) across all seven gated source files
  • PR-wide full local baseline on parent e6e08e55a: 198 suites and 3,225 tests passed; 7 suites / 141 infrastructure-dependent tests skipped; 0 failures
  • real PostgreSQL 17 migration verification: up, data, down and JSONB behavior, 4/4
  • Bicep compiled with the official compiler; only five pre-existing warnings outside this diff
  • npm audit --omit=dev --audit-level=critical: no critical findings
  • secret scan, exact bot-marker scan and git diff --check

The existing dependency tree still reports non-critical audit findings (82 high, 93 moderate, 31 low). This PR changes neither dependencies nor the lockfile; claiming a vulnerability-free repository would therefore be inaccurate.

Production activation blockers

The code and automated verification can be merge-ready, but the live rail is intentionally not activatable yet. Before enabling it, Operations must:

  • supply FRICK_BASE_URL, API key, private signing key, Bank Frick server verification key and customer number through approved secret channels
  • replace the synthetic IBANs, define CT/Operating account roles, sender priority, receive/send/sctInst flags and liquidity assets
  • register the client public key and source-IP allowlist with Bank Frick
  • seed every receiving account's cursor before setting receive=true
  • prove in the Bank Frick sandbox: signed authorization, signed empty-body GET, verified response signatures, official CAMT fixtures, CHF FOREIGN/SHA, EUR/Instant behavior, TAN/manual and exempt approval, reference echo and unique reconciliation
  • keep payout accounts receiving statements so booked debits can reconcile and release liquidity
  • enable status polling and transmission separately only after sign-off
  • monitor the shared Raiffeisen/Yapeal CAMT change as described in docs/bank-frick-operations.md

A live Bank Frick sandbox smoke test cannot be performed from repository-only inputs and has not been claimed.

References: Bank Frick WebAPI documentation and official OpenAPI specification.

Refs #4193

@joshuakrueger-dfx joshuakrueger-dfx marked this pull request as ready for review July 13, 2026 12:08
@TaprootFreak

Copy link
Copy Markdown
Collaborator

Hard requirement: 100% test coverage for the new Bank Frick code

This PR touches inbound statement import and outbound fiat payouts — money movement with double-spend exposure. For this surface, 100% test coverage of the new Bank Frick code is a hard requirement (statements, branches, functions, lines) before it can be considered ready. Nice-to-have paths and defensive throw branches are explicitly in scope — on payout code the fail-closed guards are exactly what must be proven.

The current suite is genuinely strong on the critical happy/error paths (RSA-exact-bytes signing, JWT cache + single 401 retry, fail-closed camt parsing + non-advancing watermark, idempotent-payout collision check, liquidity reservation). But coverage is not yet 100%, and right now nothing enforces it: there is no coverageThreshold, and CI runs jest --silent without --coverage, so the green checks say "tests pass", not "code is covered".

1. Make it enforceable (so it can't silently regress)

  • Add a coverageThreshold of 100% for the new Bank Frick files (branches/functions/lines/statements) to the Jest config.
  • Run coverage in CI (test:cov / jest --coverage) so the threshold is actually a gate. Without this the requirement is invisible and drift creeps back in.

2. Close the current gaps

Concrete branches/paths that still need tests:

fiat-output-job.service.ts (the finalizing state machine — highest priority):

  • getFrickStatusUpdate: the BOOKED and EXECUTED transitions and the default "Unsupported Bank Frick payment state" throw
  • checkFrickOrderStatus: the auto-approval branch, the isAvailable/DisabledProcess guards, and the IN_PROGRESS/EXECUTED/BOOKED updates
  • both catch blocks ("FRICK error" in transmitFrickPayments, "FRICK status error" in `checkFrickOrderStatus")
  • charge mapping BEN/OUR/SHA
  • createBatches Frick exclusion exercised with an actual Frick entity

frick.service.ts:

  • the approveWithoutTan-not-enabled throw in isolation (today the payoutEnabled check short-circuits it, so this guard is never asserted on its own)
  • getPaymentOrder at unit level, including the not-found throw
  • the validation throws: validateAmount, IBAN/BIC/customer validation, validateTransactionsResponse/validateAccountsResponse invalid branches, parseResponseAmount European-format path
  • JWT edge cases: exp === undefined → Infinity, malformed-JWT throw
  • the SEPA_INSTANT path + the "instant only EUR" throw
  • the moreResults ambiguity throws
  • getSafeOrderId positive path

bank-tx.service.tscheckFrickTransactions guards:

  • getBanksByName / getMultiAccounts throw handling, banks.length === 0, invalid bank.id

iso20022.service.ts: the value-date branch of assertValidCamtDate (only the booking branch is covered)

config.ts: the FRICK_PRIVATE_KEY <br>\n transform (tests currently set the PEM directly)

Once the threshold is in place and green, the 100% number is self-documenting and this requirement is met.

@TaprootFreak TaprootFreak marked this pull request as draft July 13, 2026 15:27
@TaprootFreak

Copy link
Copy Markdown
Collaborator

Follow-up: the coverage gate is in place but scoped too narrowly

Thanks for wiring up the enforcement — coverageThreshold: 100 plus the new CI step Enforce Bank Frick coverage (test:frick:cov) is exactly the right mechanism, and it addresses the "nothing enforces it" part of the earlier requirement.

The remaining gap: the gate only covers two of the new Frick files. Both the coverageThreshold block and the --collectCoverageFrom flags in test:frick:cov list only:

  • src/integration/bank/dto/frick.dto.ts
  • src/integration/bank/services/frick.service.ts

and the script runs only frick.service.spec.ts. So the gate proves 100% on the Frick client, but the following new Frick code is not in the threshold and not measured — including the highest-risk surface:

File New Frick surface left ungated Risk
supporting/fiat-output/fiat-output-job.service.ts transmitFrickPayments, getFrickStatusUpdate (BOOKED/EXECUTED/default-throw), checkFrickOrderStatus, both catch blocks, charge mapping outbound money movement / double-spend — priority 1
bank-tx/bank-tx/services/bank-tx.service.ts checkFrickTransactions watermark + guards inbound import correctness
bank/services/iso20022.service.ts value-date branch of assertValidCamtDate statement date handling
config/config.ts FRICK_PRIVATE_KEY <br>\n transform key loading

The hard requirement was 100% of the new Bank Frick code, and it's precisely the payout finalizing state machine (fail-closed guards on money movement) that most needs to be locked down. Please extend both the coverageThreshold map and the collectCoverageFrom set (and the executed spec files) to include the four files above, so the gate covers the whole surface rather than just the client.

@TaprootFreak TaprootFreak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

PR #4196 delivers a carefully engineered Bank Frick WebAPI rail: RSA-SHA512 exact-body signing, JWT caching with single-flight auth, customId-based payout recovery (including BOOKED historical lookup), dual kill switches, liquidity reservation until terminal Frick states, and shared CAMT parser hardening used by Raiffeisen/Yapeal.

Independent re-verification of the two original “bug” findings:

  • Missing fromDate on non-BOOKED lookup → downgraded to suggestion (doc default −30d is real, but PREPARED expires bank-side after 7 days unsigned; not a plausible money-loss path under normal hourly polling).
  • SEPA Instant omitting expressfalse positive / withdrawn: Bank Frick human docs forbid sending express for SEPA_INSTANT (validation error if sent); code and tests match the official example.

Remaining findings are operational hygiene, coverage-gate scope, liquidity edge cases, and shared-CAMT rollout residual risk. Client layer looks solid with focused CI coverage on frick.service.ts/frick.dto.ts.

Issue counts by severity

  • bugs: 0
  • suggestions: 6
  • nits: 1

Verification note

A separate adversarial verification pass re-checked both original severity=bug findings against code, call paths, OpenAPI, and Bank Frick human docs (incl. SEPA Instant section and PREPARED→EXPIRED in 7 days). Only the corrected severities below are posted as inline comments.

Comment thread src/integration/bank/services/frick.service.ts
Comment thread package.json Outdated
Comment thread src/subdomains/supporting/bank-tx/bank-tx/services/bank-tx.service.ts Outdated
Comment thread src/subdomains/supporting/fiat-output/fiat-output-job.service.ts Outdated
Comment thread src/subdomains/supporting/fiat-output/fiat-output-job.service.ts Outdated
Comment thread src/integration/bank/services/iso20022.service.ts Outdated
Comment thread src/integration/bank/services/frick.service.ts Outdated
@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

Round 2 addressed: full-surface coverage gate + all review suggestions

Thanks for the precise follow-up — the gate scope and every suggestion from the review are now implemented as of 0ae471757.

Coverage gate (the hard requirement). Rather than adding the four shared files wholesale to the threshold map — config.ts and bank-tx.service.ts carry large pre-existing uncovered legacy surfaces (~40–95% uncovered) that a "Bank Frick" gate should not absorb — the new Frick code was isolated into dedicated files so the gate measures exactly the surface you listed:

Surface from your table Now lives in Gated
transmitFrickPayments, getFrickStatusUpdate (BOOKED/EXECUTED/default-throw), checkFrickOrderStatus, both catch blocks, charge mapping src/subdomains/supporting/fiat-output/fiat-output-frick.service.ts 100/100/100/100
checkFrickTransactions watermark + guards (incl. getBanksByName/getMultiAccounts throws, banks.length === 0, invalid bank.id) src/subdomains/supporting/bank-tx/bank-tx/services/bank-tx-frick.service.ts 100/100/100/100
value-date branch of assertValidCamtDate src/integration/bank/services/iso20022.service.ts — raised to 100% as a whole file 100/100/100/100
FRICK_PRIVATE_KEY <br>\n transform src/config/frick.config.ts (pure buildFrickConfig(env), used by config.ts) 100/100/100/100

coverageThreshold and test:frick:cov (run by the existing CI step) now enforce 100% statements/branches/functions/lines on all six Frick files (frick.dto.ts, frick.service.ts, iso20022.service.ts, frick.config.ts, bank-tx-frick.service.ts, fiat-output-frick.service.ts) — 168 focused tests. Delegation residues left in the original files are DI wiring and one-line calls only.

Suggestions and nit:

  • fromDate (47e428967): every transactions?customId= lookup now sends fromDate=1970-01-01, and the unit tests assert it on both the active and the BOOKED lookup.
  • DELETION_REQUESTED (9293d249a): removed from the terminal set — liquidity stays reserved and polling continues until DELETED/REJECTED/EXPIRED/ERROR (or a matching debit bankTx); re-approval is asserted to never fire outside PREPARED.
  • Approval decoupling (3a428229b): isFrickAutomaticApprovalEnabled no longer references FIAT_OUTPUT_FRICK_TRANSMISSION; process gating is the caller's job (FIAT_OUTPUT_FRICK_STATUS_CHECK for the status job). Tested: transmission disabled + status check enabled still approves a PREPARED order.
  • Watermark overlap (7cdc9bba4): advances to max(previous, min(now, max bookingDate) − 2d) — monotonic, empty responses still advance (no epoch refetch on idle accounts), and the 2-day overlap deliberately re-fetches recent history with duplicates absorbed by the accountServiceRef dedup. Backfill runbook added at docs/bank-frick-operations.md.
  • Raiffeisen/Yapeal one-time duplicates: monitoring and cleanup guidance documented in the same runbook and referenced from the PR description.
  • validateCustomer (a9195fbc1): tightened to the documented OpenAPI bound ^\d{1,7}$ so misconfiguration fails closed at the config check.

Validation for 0ae471757: format/lint/type-check/build green, full local Jest run 160 suites / 2,535 tests passed (7 infrastructure-dependent suites skipped locally, they run in CI), test:frick:cov green at 100% on all six files.

@joshuakrueger-dfx joshuakrueger-dfx marked this pull request as ready for review July 14, 2026 07:42
@DFXswiss DFXswiss deleted a comment from github-actions Bot Jul 14, 2026
Automated repair triggered by Ultra Test-Agent PR Guard.

Previous head: 77eb99e
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown

⚠️ Unverified Commits (5)

The following commits are not signed/verified:

  • 5129342 fix: apply Ultra Test-Agent repair for PR feat(bank): integrate Bank Frick WebAPI #4196 (Ultra Test-Agent Fixer)
  • 5133363 fix(bank): resolve Bank Frick deep-review findings (joshuakrueger-dfx)
  • d3200b8 fix(bank): address re-review findings in the Bank Frick fix commits (joshuakrueger-dfx)
  • 7c86a87 style(bank): wrap payment observer spec helper signature for prettier (joshuakrueger-dfx)
  • 6af3006 chore(bank): isolate the Bank Frick coverage gate in its own jest config (joshuakrueger-dfx)
How to sign commits
# SSH signing (recommended)
git config --global gpg.format ssh
git config --global user.signingkey ~/.ssh/id_ed25519.pub
git config --global commit.gpgsign true

# Re-sign last commit
git commit --amend -S --no-edit
git push --force-with-lease

@TaprootFreak TaprootFreak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent deep review — 1 blocking, 11 major, 12 minor

Reviewed at head 77eb99e13 against merge-base e4bd5316b. Method: 11 parallel finder lanes over the full diff, each candidate verified adversarially by three independent lenses (refute / reproduce / impact), then a completeness critic and a second round; 49 candidates raised, 27 survived. I hand-verified every money-path finding against the production data and the pre-existing bank rows, and dropped one major candidate as empirically false (last section) — the earlier review fixes are real and I want to be precise about what does not break.

The three failure modes this repo has actually been bitten by are all clean. The migration adds no foreign key, so it cannot hit the known missing-PK drift. SettingRepository.setDateMax uses an advisory lock plus a TypeORM save() rather than a raw () => '…' expression, so there is no unquoted-camelCase Postgres crash. buildFrickConfig never throws, so deploying with no FRICK_* values cannot crash-loop the API. Both ON CONFLICT targets are backed by real constraints, and the seeded disabled-process strings match the Process enum values exactly. The default-off claim holds — this PR is safe to merge in the narrow sense that merging it does not break production today.

What it is not yet is activatable. The findings below cluster almost entirely in what happens the day Operations turn the rail on, and the first one takes the existing payout rails down with it.

Throughout, I used Yapeal as the reference implementation, since it is the closest existing analogue. Most findings below are places where Frick silently departs from the Yapeal pattern.


Blocking

1. Activating Frick breaks payouts for the bank that is already live

src/subdomains/supporting/fiat-output/fiat-output-job.service.ts:159-168

const banks = await this.bankService.getSenderBanks(currency);   // { currency, send: true }
const eligibleBanks = banks.filter();
if (eligibleBanks.length > 1 && eligibleBanks.some((c) => c.name === IbanBankName.FRICK))
  throw new Error(`Ambiguous sender bank configuration for ${currency}`);

Production today has Olkypay EUR with send=true and Yapeal CHF with send=true. The moment Operations set send=true on the Frick EUR row and enable transmission (exactly what docs/bank-frick-operations.md §3 instructs), getSenderBanks('EUR') returns two eligible banks, one of which is Frick → this throws.

It throws for every EUR fiat_output, not just the ones intended for Frick. The exception is caught by the per-entity handler in assignBankAccount() (:206), which only logs Error in fillPreValutaDate fiatOutput: <id> — so the row keeps bank = NULL / accountIban = NULL, is re-tried every minute, and is never paid out. The same applies to CHF against Yapeal. Customer sell payouts silently stop while the only signal is a repeating log line.

The kill switch does not save you either: canCreatePayments() is part of the eligibility filter, so the throw arms itself precisely when the rail is switched on.

This is a fail-closed guard that was added in response to an earlier review comment, but it encodes "two sender banks per currency is a misconfiguration" — while the PR description explicitly plans for coexistence ("sender priority … are operational inputs"). Both cannot be true.

Fix: implement the sender priority rather than throwing on plurality. A deterministic selector (an explicit priority/order column on bank, or an explicit "Frick takes precedence for currency X" rule) that returns exactly one bank and never throws for a currency that has a working incumbent. If a guard is kept, it must fire only when Frick itself is ambiguous, never when Frick merely coexists with Olkypay/Yapeal. This needs a test asserting that an EUR payout still routes to Olkypay while Frick EUR is send=true.


Major

2. Legacy Bank Frick rows make (name, currency) non-unique

migration/1783944000000-AddBankFrickPayoutTracking.js:27-36

Bank Frick was integrated once before and removed (cdd6824df Remove Bank Frick implementation (#2733), cf933e4cd), but its three bank rows were never cleaned up — production still holds Bank Frick EUR/CHF/USD (ids 1/2/3, all receive=false/send=false, carrying the old account's IBANs). Adding rows for the new account is correct — it is a genuinely different account — but it leaves ('Bank Frick', 'EUR') and ('Bank Frick', 'CHF') matching two rows each.

BankService.getBankInternal(name, currency) is a findOneCachedBy({ name, currency }) — a findOne with no ordering — so it now returns an arbitrary one of the two, cached per process. BankService.loadIbanCache() (:106-113) keeps the first row per ${name}-${currency} key, which will be the stale legacy IBAN. Any future consumer that resolves Frick by name+currency (the liquidity BankAdapter does exactly this at bank.adapter.ts:92) can silently bind to the dead account.

Fix: as part of this PR, retire the legacy rows — delete them, or mark them unambiguously (e.g. rename to Bank Frick (legacy)) so (name, currency) stays unique for the active account. Leaving two rows per currency and relying on receive/send flags to disambiguate is exactly the kind of implicit coupling that produces finding #10.

3. Synthetic IBANs are published by an unauthenticated endpoint

migration/…:31, migration/seed/bank.csv:2-3

The placeholder IBANs (LI…FRICKCHF0001 / LI…FRICKEUR0001) are mod-97 checksum-valid, so they pass every IBAN validator. GET /v1/bank is public (verified: HTTP 200, no auth) and unfiltered — BankController.getAllBanks()bankRepo.findCached('all') applies no receive/send predicate, and BankDto exposes only name/iban/bic/currency, so no consumer can distinguish a placeholder from a live account. Between merge and the Operations follow-up, that endpoint would serve five Bank Frick entries — three legacy, two invented.

Fix: the real IBANs for the new account exist. Put them in the migration and the seed directly instead of fabricating placeholders, keeping receive/send/sctInst false until sign-off — the flags, not the IBAN, are the safety mechanism. That also removes the §3 step ("replace each synthetic IBAN") entirely, which is the step most likely to be executed wrongly under pressure. Note bank.csv also assigns ids 17/18, which collide with a different bank's id in production.

4. One malformed camt.053 entry stalls the entire inbound import

src/integration/bank/services/iso20022.service.ts:246

In strict mode a single unparseable entry (an empty <EndToEndId/>, a Strd that is not CdtrRefInf) makes parseCamt053Xml throw for the whole statement. Zero transactions are returned, the watermark is not advanced, and the statement is refetched forever — so every other, well-formed customer deposit booked on that account in the same window is never imported and never credited. The only signal is a repeating log line.

Fix: fail the entry, not the statement. Separate money-critical validators (amount, currency, CdtDbtInd, status, IBAN → reject the entry) from cosmetic ones (empty optional element, unsupported Strd → drop the field). At minimum parseOptionalCamtString must treat an empty optional element as undefined.

5. Reference-less entries get a content-derived dedup key → double crediting

src/integration/bank/services/iso20022.service.ts:329

When AcctSvcrRef/TxId/NtryRef are all absent, the bank_tx identity becomes a hash over the entry's own content. Any bank-side amendment inside the 2-day overlap window (an amended AddtlNtryInf narrative, a new optional element in a future Frick camt release) changes the hash → findOneBy misses → a second bank_tx for the same real moneyassignTransactions() matches the same bank usage again → the customer is credited twice. A serialization change on the bank's side does this to every reference-less entry at once.

Fix: fail closed on a missing bank reference in strict mode (throw, or quarantine the entry) rather than synthesizing an identity from the payload. If a synthetic reference is truly unavoidable, derive it only from immutable money-identifying fields (accountIban, booking date, amount, currency, indicator, endToEndId) plus an occurrence index — never from the whole entry object.

6. Payout idempotency is a non-atomic read-before-write → double payout

src/subdomains/supporting/fiat-output/fiat-output-frick.service.ts:96-120

Nothing is persisted before the PUT /transactions. If the response is lost (gateway timeout, pod restart, response-signature failure), the catch writes only frickError and frickTxId stays NULL. The next tick re-selects the row; if the bank's transaction search does not yet return the just-created order, getPaymentOrderOrUndefined returns undefined and a second payment order is PUT for the same customId. The same double-PUT happens deterministically with two API instances: both read frickTxId IS NULL, both find no order, both PUT.

Fix: reserve the payout in the DB before any HTTP call — a conditional UPDATE fiat_output SET "frickTxId" = :customId, "isTransmittedDate" = now() WHERE id = :id AND "frickTxId" IS NULL, proceeding only if exactly one row was affected. After a send with an unknown outcome, a retry must not treat a lookup miss as "no order exists".

7. The status=BOOKED lookup can never return an order

src/integration/bank/services/frick.service.ts:337

Bank Frick's BOOKED transaction objects carry no customId and no type, but the strict validator requires both — so getPaymentOrder() throws for every settled payout, which is the normal success path. frickOrderStatus never becomes BOOKED, isApprovedDate/isConfirmedDate are never set from the status path, and the order is re-fetched and re-error-logged hourly forever. It also breaks the idempotency lookup in #6: instead of recognising an already-booked order, it raises a permanent exception. This is a regression introduced by the fromDate fix commit; a regression test feeding the spec's BOOKED example JSON through getPaymentOrder() would have caught it.

8. Booked debits include bank charges, so a charged payout never reconciles

src/integration/bank/services/frick.service.ts:148

bank_tx.amount is set to the booked CAMT entry amount (charges included) and chargeAmount is hard-coded to 0, while getUniqueOutgoingBankTx requires the amount to equal fiat_output.amount within 0.5 cents. fiat_output.charge is NULL for every row in production, so outputCharge = entity.charge ?? SHA (fiat-output-frick.service.ts:87) means every CHF Frick payout goes out SHA. A 1,000.00 CHF payout booked as Amt=1005.00, Chrgs=5.00 therefore never matches: isComplete stays false, the linked buy_fiat never completes, and — because BOOKED is deliberately non-terminal — the 1,000 CHF stays in pendingBalance permanently, shrinking the account's available balance until the rail stops readying any payout at all.

Fix: parse NtryDtls/TxDtls/Amt (falling back to AmtDtls/InstdAmt) as the amount and parse entry-level Ntry/Chrgs into chargeAmount/chargeCurrency for debit entries, as the existing SepaParser already does, instead of hard-coding chargeAmount: 0 — a silent default masking a wrong monetary value. Alternatively match on amount - chargeAmount.

9. Frick liquidity balance is never refreshed — getBalances() is dead code

src/integration/bank/services/frick.service.ts:47

BankAdapter.getForBank() has case IbanBankName.OLKY and case IbanBankName.YAPEAL — but no case FRICK, even though BankFrickService.getBalances() is fully written and has exactly the same shape as the Yapeal one (per-currency availableBalance). It has zero callers.

Following the runbook therefore yields one of two outcomes, both bad. With no LiquidityBalance row, asset.balance.amount throws a TypeError swallowed by the catch at fiat-output-job.service.ts:320no Frick payout ever gets an isReadyDate and the rail silently does nothing. If Operations seed the row by hand (the only way to satisfy the runbook), it is never updated again → every payout is gated on a permanently stale balance, and successive payouts release against the same figure until the account is overdrawn.

Fix: add the case IbanBankName.FRICK: to BankAdapter.getForBank() mirroring the Yapeal branch, and register BankFrickService with the liquidity module. Otherwise delete getBalances(), validateAccountsResponse() and the balance DTOs as dead code and correct §3 step 2, which promises a refresh that does not exist.

10. A send=true / receive=false Frick account deadlocks silently

src/subdomains/supporting/bank-tx/bank-tx/services/bank-tx-frick.service.ts:41

Reconciliation depends on the inbound receive flag: checkTransactions() skips any bank row with receive === false, so a payout-only account never gets its camt.053 imported, the booked debit never reaches bank_tx, isComplete is never set, and the liquidity reservation grows without bound until the rail stops paying out. No error, no log, no monitor — and the coupling is documented nowhere, while the account-role matrix in the runbook actively invites this configuration.

Fix: fail closed instead of relying on an operator's reading of a flag name — reject a Frick row with send === true && receive === false with a loud error (e.g. a Bank.isReconcilable getter), and state in §3 that a Frick row used for send=true must also have receive=true, with its watermark seeded first.

11. Terminally failed orders are silently abandoned and the reason is erased

src/subdomains/supporting/fiat-output/fiat-output-frick.service.ts:203

On REJECTED / EXPIRED / DELETED / ERROR the poller writes frickOrderStatus=<state>, frickError=nulldeleting the reason — releases the liquidity, stops polling, and never retries. isComplete stays false forever. The stuckFiatOutputs monitor does not look at these rows, so a customer's rejected payout sits unpaid and undetected until someone queries for it manually.

Fix: persist the failure reason instead of erasing it, and surface these rows — extend the stuckFiatOutputs monitor to count frickOrderStatus IN (<terminal>) AND isComplete = false, or raise a notification.

12. transmitPayments() destructively overwrites the customer-facing remittanceInfo

src/subdomains/supporting/fiat-output/fiat-output-frick.service.ts:122 (+ :154)

createUniqueReference() builds "DFX-FO-<id> <original>", truncates to 140 chars, and writes the result back over fiat_output.remittanceInfo. The original is kept nowhere — with a long original it is not even recoverable as a substring. The field is still read downstream: chain-report-history-dto.mapper.ts:327 uses it as the txid in the customer's chain report, which therefore changes retroactively.

The overwrite is also avoidable: frickTxId already holds the customId separately, so the matcher can key on that (or on a startsWith(customId) reference echo) without destroying the original.

Fix: keep remittanceInfo intact; store the reference actually sent to the bank in its own column.


Minor

  • fiat-output-job.service.ts:162 — an instant CHF payout can be routed to a Frick CHF row, but CHF goes out as FOREIGN and createPaymentOrder throws Bank Frick instant payments are only supported for EUR (frick.service.ts:204) → endless per-minute transmit-failure loop.
  • http.service.ts:211 — the "raw response bytes" the detached signature is verified over are not raw: axios strips a UTF-8 BOM and lossily transcodes non-UTF-8 bytes before the verifier sees them, so a legitimately signed response can fail verification permanently. Use responseType: 'arraybuffer' and verify over the Buffer.
  • frick.service.ts:468 — a failed response-signature verification (i.e. a detected tampered bank response) is reported to operators as the indistinguishable string request failed, defeating detection of the exact attack the signature check exists for. Give it a distinct error.
  • frick.service.ts:447 — no HTTP timeout on any Bank Frick request; combined with an unbounded cron lock, a single hung connection kills the payout status poller permanently and silently.
  • fiat-output-frick.service.ts:87 — the fee-bearing charge field is silently defaulted to SHA for every CHF payout (no-silent-fallback rule); the beneficiary then receives less than fiat_output.amount. Make it explicit.
  • fiat-output-job.service.ts:560 — a payout that can never be matched (or whose match is ambiguous) fails closed but is retried and re-logged forever with no escalation; liquidity stays reserved and the stuck-payout monitor is blind to it.
  • migration/…:27 — the id-less INSERT INTO "bank" is the first ever consumer of bank_id_seq in production; a lagging sequence yields a PK collision that ON CONFLICT ("iban","bic") does not catch, aborting the migration.
  • docs/bank-frick-operations.md:64 — §2 tells Operations to expect a batch of duplicate bank_tx rows on the live Raiffeisen/Yapeal accounts and to plan a manual dedup pass over real money rows, but no production code path creates bank_tx from either camt parser, so that batch cannot occur. Inviting a manual cleanup over real rows for a non-existent event is worse than saying nothing.
  • http.service.ts:24 — a stateful Bank Frick mock simulator (module-global Map, any, no return types) was added to the shared production HttpService. Wrong layer, and the state leaks across tests.
  • package.json:193 — the new 100% coverageThreshold on the shared iso20022.service.ts will turn unrelated future PRs red.
  • fiat-output.entity.ts:157 — names do not reflect content: frickTxId stores DFX's own customId, and frickOrderId stores a number as varchar(256).
  • bank.service.ts:51getSenderBank() is left behind with zero production callers.

On the coverage gate. No test executes the new raw SQL against a real Postgres (the PR ships a real-PG harness and CI already provides the DB), and no test parses a real Bank Frick camt.053 — the strict "contract" suite bypasses the XML parser entirely and the single XML fixture is hand-rolled to fit the parser. The 100% gate proves the branches execute, not that they are right: findings #7 and #8 both sit inside fully "covered" code.


Withdrawn — raised, checked, and found not to be a defect

Two of my lanes independently flagged the new isReadyDate: Not(IsNull()) filter in searchOutgoingBankTx() (fiat-output-job.service.ts:531), claiming it permanently excludes manually executed payouts of all banks from reconciliation. That is empirically false and I am dropping it. In production every fiat_output receives an isReadyDate within minutes of creation, and across the entire completed history not one row was ever reconciled without it (zero rows with outputDate IS NOT NULL AND isReadyDate IS NULL). Only two long-stuck rows would leave the candidate set. Relatedly, the old plausibility check isReadyDate > bankTx.created was not dropped — it moved into the matcher as earliestDate, alongside new amount/currency/IBAN/direction constraints. The outgoing matcher is strictly tighter than what it replaced.


Summary

The engineering standard is high, and the parts that usually go wrong here went right: the crypto boundary, the fail-closed validators, the kill switches, the watermark logic, and — importantly — the three classes of change that have actually taken this API down in the past are all avoided. Merging this does not break production.

The problem is that the rail cannot be turned on as designed. #1 stops the existing EUR/CHF payouts the moment Frick is enabled; #9 means the payout gate runs on a balance nobody refreshes; #7 and #8 mean a successful payout never reaches a terminal state or reconciles, so liquidity is consumed permanently; #10 turns a plausible account configuration into a silent deadlock. Each of these is individually invisible in production — they surface as money quietly not moving, with a log line at most.

The common thread is that Frick departs from the Yapeal pattern in exactly the places where the pattern carries the safety: no BankAdapter case, no charge parsing, no monitor coverage, and a bespoke ambiguity guard instead of a deterministic sender selection. Aligning it with Yapeal end to end would resolve #1, #8, #9 and #11 together.

@TaprootFreak

Copy link
Copy Markdown
Collaborator

Follow-up: repo precedent for bank rows — sharpens findings #2 and #3

I went back through the history for how bank rows have actually been added in this repo, because my fix proposals should follow the existing convention rather than invent one. Two things came out of it, one of which corrects the framing of my own review.

bank rows are inserted manually in production, never by a migration

There is exactly one precedent, and it was explicitly reverted:

  • 9a47168e1 feat(bank): add Yapeal EUR manual bank account CH8383019496938261612 added migration/1768943778000-AddYapealEurManualBank.js, which did INSERT INTO "bank" (…) VALUES ('Yapeal', 'CH8383019496938261612', 'YAPECHZ2', 'EUR', 0, 1, 0, 1, 405).
  • It was then removed again by f897b98a2 chore: remove migration (already inserted manually).

So the settled practice is: the row is created manually in production, and no migration touches the bank table. migration/1783944000000-AddBankFrickPayoutTracking.js is currently the only migration in the repo that inserts into bank — verifiable with grep -ri 'insert into "bank"' migration/.

That is the strongest argument for the fix I proposed in #2/#3, and it is a cleaner one than the reasoning I gave: drop the INSERT INTO "bank" from the migration entirely. Keep the fiat_output columns and the disabledProcess seeding — those genuinely belong in a migration — and let the two new Frick rows be created in production the same way the Yapeal EUR row was. That removes the fabricated IBANs from production, removes the public-endpoint exposure in #3, and removes the impossible "replace each synthetic IBAN" step from §3 of the runbook, all at once. It leaves finding #2 (the legacy rows) as the only thing still needing a decision.

The IBANs are not secret, and treating them as secret was my inconsistency

I deliberately withheld the real IBANs from my first review. That was wrong by this repo's own convention, and I want to correct it rather than quietly keep doing it:

  • migration/seed/bank.csv already carries the real Olkypay, Raiffeisen and Maerki Baumann IBANs in plaintext.
  • The real Yapeal EUR IBAN is in a commit subject line (9a47168e1).
  • Every bank IBAN is served by GET /v1/bank, which is unauthenticated — that is by design, since customers need to know where to transfer.

There is therefore no reason to encode a placeholder. The new Bank Frick account (a different account from the old, removed integration — the legacy rows in #2 carry the old account's IBANs) is:

  • EUR: LI75088110105923K000E
  • CHF: LI32088110105923K000C

Both are mod-97 valid and follow the same Frick structure as the existing rows (08811 010 + account + K000 + currency letter). These should go into migration/seed/bank.csv for fresh/dev databases — where the file already mixes real IBANs for Olkypay/Raiffeisen/Maerki with synthetic ones for Yapeal, and the real ones are the better convention — with receive/send/sctInst staying FALSE until sign-off. The flags are the safety mechanism, not a fake IBAN. Production gets the rows manually, per the precedent above.

Nothing else in the review changes.

@TaprootFreak

Copy link
Copy Markdown
Collaborator

Correction to my previous comment: the seed CSV should stay synthetic

One thing in my follow-up was wrong, and I would rather correct it than leave a bad instruction standing in the thread.

I suggested putting the real Bank Frick IBANs into migration/seed/bank.csv. Ignore that. The seed file is example data for fresh/local databases — synthetic placeholders are exactly what belongs there, and this PR already does that correctly. No change needed in bank.csv (beyond the id 17/18 collision noted in #3). That the file happens to carry real IBANs for some older banks is incidental history, not a convention worth propagating.

So the corrected position is narrower than what I wrote, and it isolates the actual defect:

  • migration/seed/bank.csv — synthetic placeholders. Correct as-is in this PR. ✅
  • migration/1783944000000-AddBankFrickPayoutTracking.js — must not insert bank rows at all. This is the one thing to change. Precedent: 9a47168e1 added a migration to insert a Yapeal bank row and it was reverted by f897b98a2 chore: remove migration (already inserted manually); no migration in the repo inserts into bank. Production rows are created manually, with the real IBANs, and never travel through the repo.

That keeps the fabricated IBANs out of production (and out of the unauthenticated GET /v1/bank) without putting real account data into an example file. Findings #1#12 are otherwise unchanged.

@TaprootFreak

Copy link
Copy Markdown
Collaborator

Closing the seed-CSV point: align the placeholders with the Yapeal pattern

To settle this concretely rather than leave it as an opinion, here is what migration/seed/bank.csv actually contains today, checked line by line against the live registry:

id name in CSV IBAN reality
18 Bank Frick LI5600000FRICKEUR0001 synthetic (this PR)
17 Bank Frick LI4200000FRICKCHF0001 synthetic (this PR)
16 Yapeal CH6783019DFXEUR000002 synthetic
15 Yapeal CH5283019DFXCHF000001 synthetic
14 Maerki Baumann LU11…5040 real — but it is the Olkypay account
13 Maerki Baumann CH48…4370 real — but it is the Raiffeisen account
12 Maerki Baumann CH77…4092 real — but it is the Raiffeisen account
8 Maerki Baumann CH38…2333 real (Maerki USD)

So it is 4 real / 4 synthetic, but that split is misleading: the four real ones are stale rows filed under the wrong bank name. They are not a convention to follow. The two rows that are actually maintained and correctly named — Yapeal, the closest analogue to Frick — are synthetic. Synthetic placeholders are right, and this PR is right to use them.

There is one detail worth aligning, though. The Yapeal placeholders keep the real bank clearing number and only fake the account part:

CH67 | 83019 | DFXEUR000002      83019 = Yapeal's real clearing number

The Frick placeholders zero that out too (LI56 | 00000 | FRICKEUR0001), so they do not resemble a Bank Frick IBAN at all — the real ones are LI.. | 08811 | …K000E. Keeping the real institution prefix is what makes a placeholder useful in dev: BIC/IBAN-consistency checks and any prefix-based routing behave like production.

Consistent with Yapeal:

18,,Bank Frick,LI17088110FRICKEUR001,BFRILI22,EUR,FALSE,FALSE,FALSE,TRUE,
17,,Bank Frick,LI35088110FRICKCHF001,BFRILI22,CHF,FALSE,FALSE,FALSE,TRUE,

Both are mod-97 valid, 21 chars (correct LI length), carry Frick's real clearing number 08811, and remain unmistakably synthetic. The ids 17/18 are fine as they are — the seed file has its own id space (next free after Yapeal's 15/16); disregard the id-collision remark in my earlier comment, which compared them against production ids that this file never sees.

This is cosmetic and does not affect any of findings #1#12. The substantive point next door stands unchanged: the migration should not insert bank rows at all (precedent f897b98a2 chore: remove migration (already inserted manually)), and the production rows carry the real IBANs of the new account, created manually.

Address the independent deep review (1 blocking, 11 major, 12 minor), aligning Bank Frick with the Yapeal reference pattern throughout.

- Sender selection: data-driven sendPriority tie-breaker instead of throwing on plurality (throw only on a genuine Frick priority tie)
- camt.053: reject malformed entries per-entry instead of failing the whole statement; fail closed on a missing bank reference in strict mode
- Payout idempotency: atomic reservation before transmit, self-heal on a definitive not-found
- BOOKED lookup no longer requires fields Bank Frick never sends; reconcile net of booked charges
- Add the Frick liquidity-balance case; reconcilability guard for send-only rows; preserve terminal failure reasons; HTTP timeouts
- Keep the customer-facing remittanceInfo (bank reference lives in frickReference); verify signed responses over raw bytes
- Migration no longer inserts bank rows; new account and legacy cleanup are documented manual production steps
@TaprootFreak

Copy link
Copy Markdown
Collaborator

Re-review of the fix commits (77eb99e13 → 5133363d8)

I re-reviewed the two follow-up commits (512934223, 5133363d8 — 28 files, +1842/−367) against my earlier findings. Method: regression-verify each of the 24 prior findings at the new head, plus a fresh adversarial sweep over the genuinely new surface (the sendPriority mechanism, frick-mock-gateway.ts, the bank.adapter FRICK case, the payment.observer change, and the cross-file column renames), every candidate then checked by three independent lenses. Prod-data claims below are verified against the live DB.

This is a strong round of fixes. The blocker and the large majority of the prior findings are genuinely resolved, not papered over. But the ~1800 lines of fix code introduced four new defects, three of them major, and one of those trips on production data the day this merges — before Frick is even enabled. Fix commits are unreviewed code; that is exactly where these landed.


Confirmed resolved (verified at head, not just claimed)

  • B1 (blocker) — sender-routing throw. Replaced by the bank.sendPriority tie-breaker; the throw now fires only for a genuine top-priority tie that includes Frick, never for an Olkypay/Yapeal tie at the shared default. Migration backfills every existing row to 1000; the runbook inserts the manual Frick rows at 2000 with the real IBANs. An EUR payout still routes to Olkypay with Frick send=true. Resolved.
  • M2/M3 — migration. Now schema-only: no bank INSERT/UPDATE/DELETE, no FK, only the fiat_output columns, bank.sendPriority, and the setting DML. The two Frick rows are a documented manual production step, matching the Yapeal precedent. Resolved.
  • M4 — one bad camt.053 entry. Per-entry onEntryRejected + fullyParsed flag; a single bad entry drops just that entry and the watermark refuses to advance past a partially-parsed window, while an account-IBAN mismatch stays fatal. Resolved.
  • M5 — reference-less dedup. Strict mode now throws missing bank reference instead of hashing the payload, so a bank-side amendment can no longer re-import the same credit. Resolved.
  • M6 — double payout. Atomic reserve update({ id, frickCustomId: IsNull() }, { frickCustomId }) with if (reserved.affected !== 1) continue before the PUT — a proper WHERE-clause mutex, and object-form (not a raw () => 'expr'), so the Postgres RHS-quoting trap does not apply. Resolved for the double-payout case (but see NEW-2 below for a stranding it introduced).
  • M7 — BOOKED lookup. validateTransactionsResponse(…, requireTypeAndCustomId) relaxes the customId-scoped lookup so a settled BOOKED order resolves instead of throwing. Resolved.
  • M8 — charged debit reconciliation. chargeAmount now parsed (not hard-coded 0) and the matcher compares amount − COALESCE(chargeAmount,0). Reconciliation resolved (but see NEW-3 for the accounting-side contradiction it exposes).
  • M9 — liquidity balance never refreshed. bank.adapter.ts now has a FRICK case wiring getBalances(), mirroring YAPEAL, with a fail-loud on a missing availableBalance. Resolved.
  • M10 — send=true/receive=false deadlock. Bank.isReconcilable getter, and it is actually called (bank-tx-frick.service.ts:44) to log a loud per-cycle error; runbook §3 documents the receive=true requirement. Detection resolved (it alerts rather than hard-blocks — a defensible choice).
  • M11 — terminal failure erased. Reason now preserved (frickError ?? 'Bank Frick order terminated: <state>') and surfaced by the observer. Resolved.
  • M12 — remittanceInfo overwrite. New frickReference column holds the bank-bound reference; remittanceInfo is only backfilled when empty, never overwritten; the matcher reads frickReference ?? remittanceInfo. Resolved.
  • m13–m16, m21, m23, m24 — instant-CHF gated to EUR; signature now verified over raw Buffer (arraybuffer); distinct FrickSignatureVerificationError; HTTP_TIMEOUT_MS; mock extracted to a typed instance-scoped FrickMockGateway; frickCustomId/frickReference/frickOrderId(64) renamed; dead getSenderBank() removed. Resolved.

Two prior minors remain unaddressed (not regressions, just untouched): m22 the 100% coverageThreshold still sits on the shared iso20022.service.ts, and m19 no test exercises the new SQL against a real Postgres (the migration spec mocks queryRunner). Both are minor.


NEW defects introduced by the fix commits

NEW-1 (major) — the observer fix flips /health to DEGRADED on deploy, from pre-existing non-Frick rows

src/subdomains/core/monitoring/observers/payment.observer.ts:126

The M11/m18 fix widened stuckFiatOutputs from a single condition to an OR of three clauses. Clause 3 — { isTransmittedDate: LessThan(Util.hoursBefore(48)), isComplete: false } — has no bank or type scope; it counts every fiat_output across all banks. health.controller.ts:149 raises DEGRADED whenever stuckFiatOutputs > 0, and that propagates to GET /health and /health/payment.

Verified against production today: the base clause matches 0 rows (health is OK now), but clause 3 already matches 4 pre-existing, non-Frick rows (2× BuyFiat, 1× BankTxReturn, 1× BuyCryptoFail — Olkypay/Yapeal, transmitted months ago, never reconciled, isComplete=false). So the moment this merges — with zero Frick activity — stuckFiatOutputs jumps 0 → 4 and /health reads DEGRADED permanently, growing as more non-Frick payouts age past 48h. A permanently-degraded signal masks real incidents and desensitises operators to the very Frick terminal-state alert (clause 2) this change adds.

Fix: scope clause 3 to Frick (frickCustomId: Not(IsNull()), or bank: { name: FRICK }), leaving cross-bank reconciliation-gap monitoring to a separately-designed metric; or close the 4 standing rows so the baseline is truly 0 before widening a shared health metric.

NEW-2 (major) — the M6 reserve strands reconciliation if the post-create persist fails

src/subdomains/supporting/fiat-output/fiat-output-frick.service.ts:120

transmitPayments atomically reserves only frickCustomId, then calls createPaymentOrder, then persists frickReference / isTransmittedDate / remittanceInfo in a separate update (:152-159). frickReference is deterministic and already computed at :97, before the reserve — but it is not folded into the atomic write. If that second update fails transiently (DB blip) or the pod restarts after the bank call succeeds, the row is left with frickCustomId set but frickReference = NULL and isTransmittedDate = NULL, while the order exists at the bank and the money moves. The self-heal in checkFrickOrderStatus only clears on FrickPaymentOrderNotFoundError; here the order is found, so frickReference is never re-derived. transmitPayments skips the row (frickCustomId set), and reconciliation keys on frickReference ?? remittanceInfo — both NULL → the bank-echoed debit can never match → isComplete never set → liquidity stranded indefinitely. That is precisely the deadlock class this PR set out to remove, reachable through a narrower window.

Fix: fold frickReference into the atomic reserve ({ frickCustomId: customId, frickReference }, both already known at :97), and/or have checkFrickOrderStatus re-persist frickReference/isTransmittedDate when it finds an existing order on a row missing them.

NEW-3 (major) — charge sign is inconsistent between reconciliation and accounting

src/subdomains/supporting/bank-tx/bank-tx/services/bank-tx.service.ts:273

The M8 fix makes Frick debits carry a real chargeAmount, and the outgoing matcher reconciles net of it (ABS((amount − COALESCE(chargeAmount,0)) − :instructed)), which only succeeds if the booked debit amount is charge-inclusive (gross = instructed + charge). But the pre-existing accounting path does the opposite: accountingAmountBeforeFee = amount + chargeAmount (and the BUY_FIAT branch accountingAmountAfterFee = amount + charge), treating amount as charge-exclusive. Both fire on the same row — a Frick BUY_FIAT payout debit is typed BUY_FIAT and fillBankTx's where: { chargeAmount: Not(IsNull()) } selects it. Before this PR Frick emitted chargeAmount = 0, so the two conventions never disagreed; the charge parsing newly activates the contradiction. A 100-CHF payout booked gross as 105 (charge 5) reconciles correctly (105−5 = 100) but is accounted as 105 + 5 = 110 — overstated by the charge on every charged Frick FOREIGN payout.

Fix: make the sign convention consistent for outgoing/debit entries — don't add chargeAmount again in accounting for debits (gate amount + chargeAmount to CREDIT entries, or subtract for DEBIT). Which of the two interpretations is correct must be confirmed against a real charged-payout CAMT from the Frick sandbox (this is already on the PR's activation-blocker list), and both paths aligned to it.

NEW-4 (minor) — self-heal race can blind the status poller

src/subdomains/supporting/fiat-output/fiat-output-frick.service.ts:56

fillFiatOutput (EVERY_MINUTE → transmitPayments) and checkFrickOrderStatus (EVERY_HOUR) get separate cron locks (dfx-cron.service.ts:54), so they can overlap. In the ~seconds-to-30s window after transmitPayments reserves frickCustomId but before the bank PUT lands, the hourly job can call getPaymentOrder → not-found → and, because its stale snapshot has isTransmittedDate = NULL, unconditionally clear frickCustomId. transmitPayments then completes the order at the bank but leaves frickCustomId NULL, so the dedicated poller (which requires frickCustomId Not null) never polls it again — a failed payout goes untracked for up to 48h. No double payout (idempotent customId + isTransmittedDate guard), but the "durable reservation" invariant is violated.

Fix: make the clear conditional — update({ id, frickCustomId: entity.frickCustomId, isTransmittedDate: IsNull() }, { frickCustomId: null, frickError: null }) — or re-load and re-check immediately before clearing.


Raised but dropped (2 of 3 verifiers refuted)

I flagged the CHF charge ?? SHA default (fiat-output-frick.service.ts:106) as a lingering silent-money-fallback (my prior m17). On adversarial re-check I'm withdrawing it: charge is a SWIFT charge-bearer code (BEN/OUR/SHA), not a money amount, and DFX already hard-codes SHA for outgoing payments elsewhere (iso20022.service.ts:650), so defaulting to SHA is an established, consistent policy rather than a masked wrong value. The fix additionally makes it durable and visible. Not a defect.


Verdict

The blocker is gone and the money-path and CAMT findings are properly fixed — this is close. What's left is that the fixes themselves need the same scrutiny the original code got: NEW-1 is the one to resolve before merge (it degrades a shared production health signal on deploy, independent of Frick, and I've confirmed it against live data), and NEW-2/NEW-3 should be fixed before the rail is activated. NEW-3 in particular can't be fully settled from the repo — it needs the real charged-payout CAMT the sandbox validation will produce.

(Method note: my automated verification pass completed the regression check of all 24 prior findings and the fresh sweep; a final completeness-critic stage aborted on an infra error, so this round did not get the extra round-2 depth the first review had. The four findings above each cleared three-lens verification and, where they touch production state, direct DB checks.)

The prior fix commits introduced four new defects the re-review caught; resolve them.

- Scope the stuckFiatOutputs health clause to Frick, so a deploy no longer reports DEGRADED from pre-existing, long-settled non-Frick payouts
- Fold frickReference into the atomic payout reservation and re-heal it in the status poller, so a crash between the two writes cannot strand reconciliation
- Make the accounting charge direction-aware (a debit amount is already gross) to match the net-of-charge outgoing reconciliation and stop double-counting the charge
- Make the not-found self-heal clear conditional, so an overlapping transmit cannot blind the status poller by clearing a just-reserved row

Regression test per defect (each fails against the pre-fix code); full suite green, Frick coverage stays 100%.
@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

Addressed the re-review of the fix commits (5133363d87c86a87da) and ran a final review over the complete PR diff.

The four new defects the re-review found in the fix commits are resolved:

  • NEW-1 — the stuckFiatOutputs health clause is now scoped to Bank Frick (frickCustomId not null), so a deploy no longer flips /health to DEGRADED from pre-existing, long-settled non-Frick payouts.
  • NEW-2frickReference is folded into the atomic payout reservation (and re-healed in the status poller when an order is found on a row still missing it), so a crash between the two writes can no longer strand reconciliation.
  • NEW-3 — the accounting charge is now direction-aware (a debit amount is treated as gross, matching the net-of-charge outgoing reconciliation), removing the double-count on charged Bank Frick payouts.
  • NEW-4 — the not-found self-heal clear is conditional (WHERE frickCustomId = <observed> AND isTransmittedDate IS NULL), so an overlapping transmit can no longer blind the status poller by clearing a just-reserved row.

Each fix has a regression test that fails against the pre-fix code. Full suite green, Bank Frick coverage stays at 100%, and the final review over the whole PR found no outstanding correctness or conformity issues.

One item to confirm before merge/activation — accounting field only, no money flow or double-payout: NEW-3 and #8 both rest on the assumption that Bank Frick books debit entries gross (charge-inclusive), which is already on the activation checklist for sandbox confirmation. Because the accounting change (amount + chargeAmount → gross for debits) lives in the shared fillBankTx, it also applies to any legacy SEPA-file debit that carries a parsed charge (SepaParser.getTotalCharge). Please confirm the gross/net convention for those legacy debits — if any were historically booked net, their accountingAmountBeforeFee would need the charge kept. No payout amount or reconciliation outcome changes; this is the internal accounting figure only.

Move the 100% coverageThreshold out of the shared package.json jest block into a dedicated jest.frick.config.js used only by test:frick:cov, so the strict per-file threshold on the shared iso20022.service.ts can no longer red an unrelated test:cov run. Behaviour-neutral for the Frick gate (same 7 files, same 100%, same CI step); also add the new root config to eslint ignores alongside the existing root configs.
@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

Follow-up consistency improvement (7c86a87da6af30068d).

Isolated the Bank Frick 100% coverage gate into its own jest.frick.config.js (used only by test:frick:cov), out of the shared package.json jest block. This addresses the earlier note that a strict per-file threshold on the shared iso20022.service.ts would otherwise red an unrelated test:cov run — no other bank integration ships a coverage gate in the shared config.

Behaviour-neutral for the Frick gate: same seven files, same 100%, enforced in the same CI step. Verified directly — dropping a covered spec still reds the gate (iso20022.service.ts → 57.72% → threshold not met), and test/test:cov no longer carry the per-file requirement. No production code changed; the new root config is added to eslint ignores alongside the existing root configs.

A separate dead-guard cleanup was considered and deliberately not taken — that guard turned out to be a tested defense-in-depth layer on the reconciliation path, so it was kept.

@TaprootFreak TaprootFreak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. I re-verified the fix commit (d3200b8ed) against my three re-review findings against the code (and the CAMT parser) — all three are correctly resolved.

  • NEW-1 (health DEGRADED on deploy): stuckFiatOutputs clause 3 is now scoped with frickCustomId: Not(IsNull()). Pre-existing non-Frick rows (frickCustomId NULL) no longer match, so /health won't flip to DEGRADED on deploy, while genuine transmitted-but-stuck Frick payouts (>48h) are still caught. The observer spec evaluates the real clause array and fails against the pre-fix generic clause — not tautological.
  • NEW-2 (reconciliation strand): frickReference is folded into the atomic reserve ({ frickCustomId: customId, frickReference }) and removed from the deferred post-create write, so a crash between the two writes can no longer leave frickReference NULL while the order is live at the bank. The status-poller gap-heal recomputes the identical deterministic reference (frickCustomId is only ever the deterministic customId), so it cannot diverge.
  • NEW-3 (charge sign): Confirmed against iso20022.service.ts:289-294 — a charge is parsed only on DEBIT entries and the booked DEBIT amount is gross (charge-inclusive). The matcher subtracts chargeAmount for net matching, and accounting no longer adds it back on debits (accountingCharge = CREDIT ? chargeAmount : 0). Matcher and accounting are consistent and correct to the actual parser convention; non-Frick banks (chargeAmount 0) are unaffected. Substitution is complete across all three branches.
  • NEW-4: the not-found self-heal clear is now conditional — a clear improvement.

One residual, minor, non-blocking for merge — but must be fixed before the rail is armed:
The conditional not-found clear guards on { frickCustomId, isTransmittedDate: IsNull() }. That still matches a row a concurrent minutely transmitPayments has reserved-and-PUT but not yet stamped with isTransmittedDate. In that narrow window (overlapping hourly poll + minutely transmit) the clear can null frickCustomId on a row whose order is already live at Bank Frick, stranding it — and it would be invisible to all three stuckFiatOutputs clauses. It is fail-closed at the bank (createPaymentOrder's lookup-before-PUT + assertSamePayment prevent any double payment) and strictly better than the pre-fix unconditional clear, so it is not a regression.

Note the naive fix (guard on frickReference IS NULL) does not work: the reserve now always sets frickReference, so that guard would never release a genuinely-failed reservation. A durable fix needs a marker that distinguishes "being transmitted right now" from "reserved but abandoned" — e.g. stamp a transmitting-marker in the reserve write and have the clear exclude it, or re-check order existence immediately before the clear.

Because this is only reachable once FRICK_PAYOUT_ENABLED=true, it is safe to merge and deploy now (arms nothing) and close before arming.

CI green on 6af30068 (incl. Build and test).

@TaprootFreak TaprootFreak merged commit 5ad7f62 into develop Jul 15, 2026
8 checks passed
@TaprootFreak TaprootFreak deleted the agent/integrate-bank-frick branch July 15, 2026 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants